Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: skip validation for server decompressed objects #1063

Merged
merged 8 commits into from
Feb 6, 2020
Merged

fix: skip validation for server decompressed objects #1063

merged 8 commits into from
Feb 6, 2020

Conversation

jkwlui
Copy link
Member

@jkwlui jkwlui commented Feb 6, 2020

Fixes #709

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 6, 2020
@crwilcox
Copy link
Contributor

crwilcox commented Feb 6, 2020

Thanks @jkwlui . I am going to make a small tweak to the TODO and get CI Green

@codecov
Copy link

codecov bot commented Feb 6, 2020 •
edited
Loading

Codecov Report

Merging #1063 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1063      +/-   ##
==========================================
+ Coverage   99.01%   99.01%   +<.01%     
==========================================
  Files          11       11              
  Lines       10708    10721      +13     
  Branches      473      475       +2     
==========================================
+ Hits        10602    10615      +13     
  Misses        106      106
Impacted Files Coverage Δ
src/file.ts 99.65% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcceece...546dfe3. Read the comment docs.

@crwilcox crwilcox added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 6, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 6, 2020
frankyn
frankyn approved these changes Feb 6, 2020
View reviewed changes
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

stephenplusplus
stephenplusplus suggested changes Feb 6, 2020
View reviewed changes
src/file.ts Outdated Show resolved Hide resolved
system-test/storage.ts Outdated

const file = bucket.file(filename);
file
.createReadStream()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an on-error handler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different from the one on line 2269?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that one will catch errors from the write stream, but if the read stream has an error, it would go uncaught currently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like @jkwlui addressed this?

feywind
feywind reviewed Feb 6, 2020
View reviewed changes
system-test/storage.ts Outdated
it('should skip validation if file is served decompressed', done => {
const filename = 'logo-gzipped.png';
bucket
.upload(FILES.logo.path, {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: style comments - the mix of promises and callbacks makes me twitch a bit. Actually the preferred way now is to use async/await, so you could make the lambda passed to it() async and then await on stuff. I think there's a promisify function we have handy for tmp?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for unit tests, I'd be tempted to just use mktempSync, I don't know that we need a module for this:

https://nodejs.org/api/fs.html#fs_fs_mkdtempsync_prefix_options

I'd also consider switching to to async/await, would probably unravel this a bunch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up using tmp.fileSync() since it's already there :)
Probably gonna require more boilerplate code if we'd switch to using fs.mkdTempSync.

bcoe
bcoe reviewed Feb 6, 2020
View reviewed changes
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is looking pretty reasonable to me, main comment is the same as @feywind's, I'd try to avoid mixing promises and functions; in tests using sync methods is fine to reduce callback nesting.

src/file.ts Outdated Show resolved Hide resolved
system-test/storage.ts Outdated
it('should skip validation if file is served decompressed', done => {
const filename = 'logo-gzipped.png';
bucket
.upload(FILES.logo.path, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for unit tests, I'd be tempted to just use mktempSync, I don't know that we need a module for this:

https://nodejs.org/api/fs.html#fs_fs_mkdtempsync_prefix_options

I'd also consider switching to to async/await, would probably unravel this a bunch.

@jkwlui jkwlui added the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 6, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Feb 6, 2020
@crwilcox crwilcox added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 6, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 6, 2020
@stephenplusplus stephenplusplus merged commit d6e3738 into master Feb 6, 2020
@jkwlui jkwlui deleted the skip-validation branch February 6, 2020 23:54
jkwlui added a commit that referenced this pull request Feb 10, 2020
@jkwlui @crwilcox
fix: skip validation for server decompressed objects (#1063)
a0d9f4f 
* fix: skip validation for server decompressed objects

* fix: formatting changes to todo comment for tracking

* fix: lint

* fix: remove unnecessary var set

* use async/await

* listen to response evt on read stream

* npm run fix

Co-authored-by: Christopher Wilcox <crwilcox@google.com>
@shaffeeullah shaffeeullah mentioned this pull request Nov 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feywind feywind feywind left review comments

@crwilcox crwilcox crwilcox left review comments

@bcoe bcoe bcoe left review comments

@frankyn frankyn frankyn approved these changes

@stephenplusplus stephenplusplus stephenplusplus approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CONTENT_DOWNLOAD_MISMATCH with successful file download
8 participants
@jkwlui @crwilcox @bcoe @stephenplusplus @frankyn @feywind @googlebot @yoshi-kokoro